Skip to content

Make Storage::read_slice take a mutable slice#486

Closed
ia0 wants to merge 2 commits intogoogle:developfrom
ia0:read_mut
Closed

Make Storage::read_slice take a mutable slice#486
ia0 wants to merge 2 commits intogoogle:developfrom
ia0:read_mut

Conversation

@ia0
Copy link
Copy Markdown
Member

@ia0 ia0 commented Jun 2, 2022

As mentioned in this comment, the Storage::read_slice API is too strong because some implementations cannot return a slice. This PR changes the API to take a mutable slice instead. This essentially enables 2 use-cases:

@ia0 ia0 force-pushed the read_mut branch 2 times, most recently from 3a26a18 to 6776248 Compare June 2, 2022 10:56
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 2, 2022

Coverage Status

Coverage decreased (-1.3%) to 88.617% when pulling 773f3a3 on ia0:read_mut into 85fe9cd on google:develop.

@ia0 ia0 changed the title [Test] Make Storage::read_slice take a mutable slice Make Storage::read_slice take a mutable slice Jun 2, 2022
@ia0 ia0 requested a review from kaczmarczyck June 2, 2022 12:12
// The slice spans the next page.
let index = pos.next_page(&self.format).index(&self.format);
result.extend_from_slice(self.storage_read_slice(index, length - max_length));
self.storage_read_slice(index, &mut result[max_length as usize..]);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking: I was wondering how much more copying we will have to do with this PR. Looks like we have to copy up to 2 pages now, instead of just the final slice that has always been a copy?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there will be more allocations and more copying with this PR.

However, for the specific code on which this comment is attached, there's actually no additional allocation or copy. That's probably one of the rare exceptions. For me the most problematic place would be when reading a page just to check that it's erased (this doesn't happen often, at boot and compaction) and when writing a slice to check that the write is valid (this happens at store mutation). When reading, we don't have additional penalty, which is actually ironic since the PR is only changing the reading API. This comes from the fact that the store API is already allocating and copying, so we don't add more. The allocation and copying during store mutation (insert, remove, compaction, etc) is acceptable because it's dominated by the cost of the actual flash write. However the allocation and copying for booting the store is more problematic. But since it happens at boot time, the extra cost is less sensitive.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a store benchmark you can quickly run, out of curiousity?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I forgot about it. I've updated the numbers. Here's a summary:

  • Boot is 35% slower
  • Compaction is 5% slower
  • Insert is 30% slower
  • Remove is 35% slower

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that may not be problematic, how hard would it be to have an API that is still general over different use cases (files and embedded), but less performance issues on embedded? Just to understand the alternative.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, there's an alternative that we may prefer: #487 .

I believe there would be an ideal alternative. I don't think it's hard to implement, so I might just try.

@kaczmarczyck kaczmarczyck self-requested a review June 2, 2022 14:45
kaczmarczyck
kaczmarczyck previously approved these changes Jun 2, 2022
@ia0
Copy link
Copy Markdown
Member Author

ia0 commented Jun 3, 2022

Closing in favor of #487

@ia0 ia0 closed this Jun 3, 2022
@ia0 ia0 deleted the read_mut branch June 3, 2022 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants